Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Splicing support #95

Draft
wants to merge 13 commits into
base: 2023-08-remote-hsmd-v23.08
Choose a base branch
from

Conversation

ksedgwic
Copy link

@ksedgwic ksedgwic commented Sep 6, 2023

Upstream CLN support for VLS

@ksedgwic ksedgwic marked this pull request as draft September 6, 2023 15:57
@ksedgwic ksedgwic force-pushed the 2023-08-explore-splicing branch 2 times, most recently from b8b6b15 to f3da7ba Compare September 6, 2023 22:49
local_upfront_shutdown_script,
local_upfront_shutdown_wallet_index,
&peer->channel->basepoints[REMOTE],
&peer->channel->funding_pubkey[REMOTE],
Copy link
Collaborator

@devrandom devrandom Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the funding pubkey is allowed to vary in the splice, and we need to be involved in that. I'm wondering where that happens and how we will be informed of the remote one (and asked about the local one).

(note that the funding keys will be the same for all splice candidates)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to be involved in that

Are you saying that roundtrips/protocol is missing?

how we will be informed of the remote one

I think we are being informed with this call. Am I missing something?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

funding pubkey is allowed to vary in the splice

https://github.com/lightning-signer/c-lightning/blob/2023-08-explore-splicing/channeld/channeld.c#L3536-L3539

That seems to imply it is not changing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

each of the prior is one side, presume there are similar in the other "direction"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the spec allows for it to change (for the whole splice, not per RBF candidate). we should ask the CLN team about this.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just scoured the spec looking for where it is allowed to change ... can you point me to it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/lightning/bolts/pull/863/files#diff-ed04ca2c673fd6aabde69389511fa9ee60cb44d6b2ef6c88b549ffaa753d6afeR522

    type: 74 (splice)

    data:
        [chain_hash:chain_hash]
        [channel_id:channel_id]
        [u64:funding_satoshis]
        [u32:funding_feerate_perkw]
        [u32:locktime]
        [point:funding_pubkey]

    type: 76 (splice_ack)

    data:
        [chain_hash:chain_hash]
        [channel_id:channel_id]
        [u64:funding_satoshis]
        [point:funding_pubkey]

Copy link
Collaborator

@devrandom devrandom Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like CLN explicitly doesn't support changing them (they send an error to the peer if the key changes), but I assume that will be fixed before release. need to ask them.

@@ -3586,6 +3619,8 @@ static void splice_accepter(struct peer *peer, const u8 *inmsg)
new_inflight->last_tx = NULL;
new_inflight->i_am_initiator = false;

update_hsmd_with_splice(peer, new_inflight);
Copy link
Collaborator

@devrandom devrandom Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the amounts are in peer->splicing->*_relative

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in this case the amount that the hsmd needs to know is the total amount, right?

We can calculate by the current funding tx amount + remote one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to specify two amounts in setup_channel that the splice affects:

  • the total channel amount, which is the size of the funding outpoint
  • the push_val, which expresses how much of the channel value should be on the non-originating side.

I think this should be the current channel balance of the fundee (non-originator) + amount of additional sats added (signed, possible negative) by the fundee.

Does this seem correct?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me, just to rephrase in other words, the signer with the setup_channel needs to know the current total amount of the channel and then also
a relative amount (push_val) that we are adding with the splice.

This looks good to me.

I also noted that I made a typo in my previous message, the formula that I was suggesting is current funding tx amount + relative amount

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, push_val is the counterparty's value in the channel. sorry, it's a bit confusing:

  • holder (local) value is channel_val - push_val (let's call that holder_val for clarity)
  • counterparty value is push_val (let's call that counterparty_val for clarify)
  • these are absolute values
  • to convert to deltas from previous values (before the current splice), you have to compute the difference for each side:
    • e.g. holder_delta = holder_val_current - holder_val_previous

@devrandom
Copy link
Collaborator

the location of the calls looks reasonable, but a couple of open questions above

When BOLT-2 renamed funding_locked to channel_ready the hsmd message
named ready_channel became very confusing.  Change it to setup_channel
instead to not confuse developers.not trigger the wrong association.

Because the hsmd wire messages are parsed by number this change is
forward and backwards compatible.

Changelog-Changed: hsmd: Renamed hsmd_ready_channel to hsmd_setup_channel because of the BOLT-2 rename of funding_locked to channel_ready
@ksedgwic ksedgwic force-pushed the 2023-08-explore-splicing branch 5 times, most recently from b321031 to ae30195 Compare September 27, 2023 01:17
@ksedgwic ksedgwic force-pushed the 2023-08-explore-splicing branch 4 times, most recently from 5a7bf3e to 14be753 Compare September 28, 2023 03:36
ksedgwic referenced this pull request in vincenzopalazzo/lightning Sep 29, 2023
The signer needs to know when the splice operation starts and the
splice parameters for each splice transaction candidate.

The channel establishment v2 (dual funding) code path already
notifies the signer via the hsmd API hsmd_ready_channel calls
However, the splicing code path does not.

Link: ElementsProject#6723
Suggested-by: @devrandom
Co-Developed-by: Ken Sedgwick <[email protected]>
Signed-off-by: Vincenzo Palazzo <[email protected]>
@ksedgwic ksedgwic force-pushed the 2023-08-explore-splicing branch from 14be753 to e873db1 Compare October 6, 2023 00:12
ksedgwic and others added 10 commits October 5, 2023 17:16
The signer needs to know when the splice operation starts and the
splice parameters for each splice transaction candidate.

The channel establishment v2 (dual funding) code path already
notifies the signer via the hsmd API hsmd_ready_channel calls
However, the splicing code path does not.

Link: ElementsProject#6723
Suggested-by: @devrandom
Co-Developed-by: @devrandom
Co-Developed-by: Ken Sedgwick <[email protected]>
Signed-off-by: Vincenzo Palazzo <[email protected]>
Revert "splicing: Skip splice test until splicing is supported"

This reverts commit bd9494c.
…ect#6722])

Changelog-Added: Added hsmd_check_outpoint and hsmd_lock_outpoint per ([ElementsProject#6722])
@ksedgwic ksedgwic force-pushed the 2023-08-explore-splicing branch from e873db1 to b2dcf3b Compare October 6, 2023 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants